Allow newer dotnet cli #633
Conversation
TylerLeonhardt
requested review from
daviwil,
rkeithhill,
SeeminglyScience and
rjmholt
| @@ -37,7 +37,7 @@ task SetupDotNet -Before Clean, Build, TestHost, TestServer, TestProtocol, TestP | |||
| } | |||
|
|
|||
| # Make sure the dotnet we found is the right version | |||
| if ($dotnetExePath -and (& $dotnetExePath --version) -eq $requiredSdkVersion) { | |||
| if ($dotnetExePath -and [version](& $dotnetExePath --version) -ge [version]$requiredSdkVersion) { | |||
There was a problem hiding this comment.
So why don't we just use a global.json file to specify the SDK version? See https://github.com/PowerShell/PowerShell/blob/master/global.json
There was a problem hiding this comment.
@rkeithhill that's kinda opposite of why I opened this PR. I didn't want to require 1 particular SDK version. I wanted to be able to build using the dotnet sdk of any version greater than 2.0.
Ex. I have 2.1.4 on my machine and it builds just fine.
By adding a global.json it forces the construct of "you need this exact version" at least until this is implemented:
https://github.com/dotnet/core-setup/issues/679
There was a problem hiding this comment.
I was just looking for a mechanism to set a minimum version, and lo and behold it led me right back here from that core-setup issue
There was a problem hiding this comment.
It's so sad. I just want to not have to download a million dotnet sdks 😭
There was a problem hiding this comment.
I don't know if it is dl of dotnet or what but every time I debug the extension, it builds PSES and that part is sloooow. :-(
There was a problem hiding this comment.
dotnet build times have supposedly improved noticeably in the 2.1 preview. Maybe I should try that version to see how it performs.
There was a problem hiding this comment.
dotnet 2.1 seems good to me :)
this PR will allow you to use that build if you have it on your machine already... but still be compat with 2.0 if folks have that
There was a problem hiding this comment.
@rkeithhill is that thumbs up a sign off 😉
There was a problem hiding this comment.
BTW there are a bit too many warnings IMO. At some point, we should clean up what we can. I see a couple of variable not used warnings that should be easy to fix.
There was a problem hiding this comment.
Agreed. The unused variable warnings would make a great Up-for-grabs I think.
our build script required 2.0.0. Now the cli is up to 2.1.4. This small change allows newer versions to be used.
We may want to eventually put an upper bound on this... but I don't think the dotnet cli will change much at least how we're using it.